Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

(CAT-1226) - Remove Compatibility for Puppet 7.10 and below #73

Merged
merged 4 commits into from
Oct 4, 2023
Merged

(CAT-1226) - Remove Compatibility for Puppet 7.10 and below #73

merged 4 commits into from
Oct 4, 2023

Conversation

jordanbreen28
Copy link

@jordanbreen28 jordanbreen28 commented Oct 2, 2023 •
edited
Loading

Summary

This PR:

  • Removes compatibility with Puppet versions 7.10 and below

Additional Context

Contains commits from #68, thanks to @ekohl.

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified.

ekohl
ekohl reviewed Oct 2, 2023
View reviewed changes
Copy link

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had hoped you'd take #68 as a base.

@jordanbreen28
Copy link
Author

I had hoped you'd take #68 as a base.

Apologies... I was actually in the process of commenting on your PR as your comment popped up on this! I was rather silly and hadn't checked for other PRs before commencing the work. Looking to incorporate most of the changes you have in your PR into this one so happy to collaborate

@ekohl
Copy link

ekohl commented Oct 2, 2023

#68 (comment) did raise a valid concern with my approach.

I think #67 would be better to merge first. I just rebased it and updated it to make RuboCop pass. Let's see if CI agrees.

@jordanbreen28 jordanbreen28 force-pushed the CAT-1226-address_outdated_code branch 6 times, most recently from fc65ffe to 7a27b79 Compare October 2, 2023 10:53
@jordanbreen28 jordanbreen28 changed the title (CAT-1226) - Remove Compatibility Puppet 6.x and below (CAT-1226) - Remove Compatibility for Ruby 1.x, , Rspec 2 & Puppet 6.x and below Oct 2, 2023
@jordanbreen28 jordanbreen28 changed the title (CAT-1226) - Remove Compatibility for Ruby 1.x, , Rspec 2 & Puppet 6.x and below (CAT-1226) - Remove Compatibility for Ruby 1.x, Rspec 2 & Puppet 6.x and below Oct 2, 2023
@jordanbreen28 jordanbreen28 changed the title (CAT-1226) - Remove Compatibility for Ruby 1.x, Rspec 2 & Puppet 6.x and below (CAT-1226) - Remove Compatibility for Ruby 1.x, Rspec 2 & Puppet 7.11 and below Oct 2, 2023
@jordanbreen28 jordanbreen28 force-pushed the CAT-1226-address_outdated_code branch 4 times, most recently from 8e93334 to 79c7724 Compare October 2, 2023 12:36
@jordanbreen28 jordanbreen28 marked this pull request as ready for review October 2, 2023 12:38
@jordanbreen28 jordanbreen28 requested review from bastelfreak and a team as code owners October 2, 2023 12:38
@jordanbreen28
Copy link
Author

@ekohl is there any particular reason you opted to setting minimum compatible puppet version to 7.11? We need to support all versions of the two most recent major releases, so will probably need to reduce the minimum supported puppet version to 7.0.

@bastelfreak
Copy link
Collaborator

Why do you need to support 7.0? That version is dead and won't receive updates anymore? Even the CAT team doesn't support it in most of Puppetlabs modules because it doesn't support the array datatype for exec commands.

@ekohl
Copy link

ekohl commented Oct 2, 2023

@jordanbreen28 I'd still merge #67 first and not include it here. The PR is already hard enough to review, so that's why I split it up.

As for the minimum version, it's for the Puppet.runtime[:facter] feature that landed in 7.11. The code now expects that to work, rather than having some fallback code.

@bastelfreak does raise a point that Puppetlabs itself already takes 7.9 as a lower bound, even if they don't acknowledge it. Like in puppetlabs/puppetlabs-apache@186c729 I had to make it explicit and my comment in puppetlabs/puppet-lint#142 (review) was ignored when merging it (that check was reverted, so doesn't apply anymore).

@jordanbreen28
Copy link
Author

@bastelfreak @ekohl I'm going to raise this internally, I'll update the thread when I've more info!

That sounds good to me @ekohl , I'll work to get #67 merged and this PR rebased onto it.

@jordanbreen28 jordanbreen28 force-pushed the CAT-1226-address_outdated_code branch 5 times, most recently from 662e7d9 to 902fb76 Compare October 2, 2023 13:38
@jordanbreen28
Copy link
Author

@bastelfreak @ekohl thanks for your help with this one, we've decided to go ahead and add compatibility from 7.11.0 onwards.
This is ready for any reviews now.

@jordanbreen28 jordanbreen28 mentioned this pull request Oct 2, 2023
Closed
@jordanbreen28 jordanbreen28 force-pushed the CAT-1226-address_outdated_code branch 2 times, most recently from c135d77 to ef01d40 Compare October 2, 2023 16:27
nabertrand
nabertrand reviewed Oct 2, 2023
View reviewed changes
README.md Show resolved Hide resolved
docs/documentation/configuration/index.md Outdated Show resolved Hide resolved
ekohl
ekohl suggested changes Oct 3, 2023
View reviewed changes
Copy link

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the impression you took my commit and claimed ownership of it by resetting the author. I'm assuming this is a mistake, rather than intentional but please make sure my original authored code is properly attributed in the git history.

@jordanbreen28
Copy link
Author

I get the impression you took my commit and claimed ownership of it by resetting the author. I'm assuming this is a mistake, rather than intentional but please make sure my original authored code is properly attributed in the git history.

Authoring got lost somewhere in the rebasing. I am working to fix this.

@jordanbreen28
Copy link
Author

@ekohl I hope thats fixed now, apologies!

@ekohl @jordanbreen28
(CAT-1226) - Raise minimum version to Puppet 7.11
316d959 
This avoids the need to have any version specific code and it's the only
thing that's testing in CI.
@GSPatton GSPatton merged commit 5122a70 into main Oct 4, 2023
6 checks passed
@GSPatton GSPatton deleted the CAT-1226-address_outdated_code branch October 4, 2023 12:58
@jordanbreen28 jordanbreen28 changed the title (CAT-1226) - Remove Compatibility for Puppet 7.11 and below (CAT-1226) - Remove Compatibility for Puppet 7.10 and below Oct 9, 2023
@jordanbreen28 jordanbreen28 mentioned this pull request Oct 11, 2023
Merged
5 tasks
bastelfreak added a commit to bastelfreak/voxpupuli-test that referenced this pull request Oct 11, 2023
@bastelfreak
dont set trusted_node_data/trusted_server_facts
3077c19 
The setting got removed in rspec-puppet 4:

puppetlabs/rspec-puppet#73
@bastelfreak bastelfreak mentioned this pull request Oct 11, 2023
Merged
@smortex smortex mentioned this pull request Nov 2, 2023
Closed
smortex added a commit to smortex/onceover that referenced this pull request Nov 2, 2023
@smortex
Fix compatibility with rspec-puppet 4+
9a513ab 
These settings where used for compatibility with Puppet 7.10 and older
(Puppet 7.10 was released more than 2 years ago).  rspec-puppet 4
removed them:
puppetlabs/rspec-puppet#73

In order to avoid this kind of accident in the future, add an upper
bound on the rspec-puppet dependency.  Version 5.0.0 is also working
with this change, so accept anything before 6.0.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak left review comments

@nabertrand nabertrand nabertrand left review comments

@ekohl ekohl ekohl requested changes

@GSPatton GSPatton GSPatton approved these changes

Assignees
No one assigned
Labels
backwards-incompatible hacktoberfest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jordanbreen28 @ekohl @bastelfreak @nabertrand @GSPatton